Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: remove pubspec_overrides.yaml and pubspec_overrides.yaml.disabled #2262

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

EchoEllet
Copy link
Collaborator

@EchoEllet EchoEllet commented Sep 21, 2024

Description

Remove and disable the local development mode completely.

The file pubspec_overrides.yaml exist so we can override the dependencies to use the local ones instead from pub.dev

We usually don't want that in production, so it's only during development to allow contributors to focus on the changes instead having to update the minimum change of flutter_quill by first introducing the internal breaking changes, upload a version so we can use it in the version after it as a minimum for flutter_quill_extensions (See 10.7.2) as an example.

Contributors had to include the dependency_overrides and then remove it before pushing a commit, sometimes they push a commit to test the changes in CI. I introduced the pubspec_overrides.yaml.disabled which will be ignored by pub.dev (as it uses pubspec_overrides.yaml instead) and two scripts which will create pubspec_overrides.yaml files and delete them (which is ignored in .gitignore).

I don't see a strong reason for this feature, if we need to fix something we should do it manually and ensure a compatible minimum version requirement, we also usually test using the example app so it's usually enough to have it there.

Also, I see some other issues of having only one version for all packages, dart_quill_delta should have its own version separated from flutter_quill. Same to flutter_quill_test as it didn't introduce any changes. The flutter_quill and flutter_quill_extensions might or might not require only one version.

  • When we want to introduce breaking changes to one package, we have to break them all
  • When a minor version made to a package, we have to publish them all over again
  • They will all share the same CHANGELOG.md which is not very organized (see Improve quality and format of CHANGELOG.md #2211).
  • It gives us some limitations when introducing internal breaking changes. We will always have to make some internal changes backward compatible between flutter_quill and flutter_quill_extensions. When a new API introduced in flutter_quill and it's needed in flutter_quill_extensions, with our current workflow, we have to release it in backward compatible way even if it's an internal API first, once we have the new version of flutter_quill, we need another release that increases the minimum version of flutter_quill in flutter_quill_extensions
  • The current workflow doesn't give us the flexibility when publishing the packages in the correct order, there is no specific order that we must stick to.
  • We don't want to fix this issue between flutter_quill and flutter_quill_extensions, when a breaking change is introduced, users expect improvements and new features to the final product, so we should enhance flutter_quill_extensions and once there is a strong reason to introduce a breaking change by fixing issues, introducing features and more tests then we will probably introduce it, fixing it requires a separate package of flutter_quill_extensions and that's not a priority at the moment.
  • This whole workflow is not scalable enough to have quill_native_integration with its own packages (quill_native_integration_platform_interface, quill_native_integration_web, quill_native_integration_windows etc...). See feat: Use quill_native_bridge as default impl in DefaultClipboardService, fix related bugs in the extensions package #2230 for more details.
  • We don't want complexity that doesn't fix a issue, especially with our current situation.

As for quill_native_bridge, I will also separate the version, I need to publish a new package and deprecate this, luckily it's already experimental and for internal use so shouldn't be a breaking change as we didn't expose the APIs of it yet (see #2230). The new package will probably be quill_native_integration.

Tip

This PR only removes thee local development mode and those two files in addition to their scripts and doesn't fix or change all of the points mentioned above. The publishing will still be the same.

Related Issues

Type of Change

  • New feature: Adds new functionality without breaking existing features.
  • 🛠️ Bug fix: Resolves an issue without altering current behavior.
  • 🧹 Code refactor: Code restructuring that does not affect behavior.
  • Breaking change: Alters existing functionality and requires updates.
  • 🧪 Tests: Adds new tests or modifies existing tests.
  • 📝 Documentation: Updates or additions to documentation.
  • 🗑️ Chore: Routine tasks, or maintenance.
  • Build configuration change: Changes to build or deploy processes.

@EchoEllet
Copy link
Collaborator Author

EchoEllet commented Sep 21, 2024

@CatHood0 , @AtlasAutocode , @singerdmx
If you also think having this feature of local development mode is unnecessary and can be confusing, will merge this PR to remove it.

Edit: Merged as I want to focus on more important issues, we can revert if you prefer.

@EchoEllet EchoEllet marked this pull request as ready for review September 21, 2024 15:08
@EchoEllet EchoEllet merged commit 9aa4e6c into master Sep 21, 2024
2 checks passed
Copy link
Collaborator

@CatHood0 CatHood0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found it useful when adding new changes to, for example, flutter_quill_extensions and flutter_quill and ensuring that any mutual changes are not ignored. However, it is confusing in itself since it is almost magical to run a script and be able to have the local changes reflected without having to update one part and then the other part that needs to have that new change.

In any case, it seems to me to be quite useful, but only in some cases. Most of the time it doesn't help much as we are more focused on fixing bug or stability issues than adding new features that require local mutual changes to be reflected.

@EchoEllet EchoEllet deleted the chore/remove-pubspec-overrides-yaml-disabled branch September 21, 2024 15:10
@EchoEllet
Copy link
Collaborator Author

EchoEllet commented Sep 21, 2024

At some point, we may consider using tools like melos to manage multiple packages, until now shouldn't be an issue.

Also having two files in the file tree can contribute to the project structure complexity.

@EchoEllet
Copy link
Collaborator Author

EchoEllet commented Sep 21, 2024

See an example issue caused after merging this change to #2230. We need to publish changes of quill_natibe_bridge before proceeding further.

This will ensure such issues will not be encountered by users, as we may forget to update the minimum required version. It seems that removing this feature was a bit of a hasty decision.

@EchoEllet
Copy link
Collaborator Author

An alternative solution is to add pubspec_overrides.yaml temporarily only when needed and revert later once is no longer required. See this commit as an example.

This will also help ensure that we do publish the package that needs to be published first before updating the other and bump the minimum version of that package in the dependencies.

@CatHood0
Copy link
Collaborator

An alternative solution is to add pubspec_overrides.yaml temporarily only when needed and revert later once is no longer required. See this commit as an example.

Thanks for mentioning this. I'll keep it in mind for when I need it.

CatHood0 pushed a commit to CatHood0/flutter-quill that referenced this pull request Sep 21, 2024
@EchoEllet
Copy link
Collaborator Author

I'm able to reproduce the issue on my local machine

It seems that melos does address that kind of issue more effectively. See Why do I need to bootstrap

@CatHood0
Copy link
Collaborator

It seems that melos does address that kind of issue more effectively.

For now I'll first look at how to use melos and slowly implement it in my packages. Once I'm confident that I can configure it without messing anything up, I'll open an issue or discussion to implement melos in the package.

@EchoEllet
Copy link
Collaborator Author

Sure, before introducing this solution, I planned on trying to use melos but didn't since it required contributors to install it, which is an additional step that I wanted to avoid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants